GH-50515: [C++][Compute] Respect parent validity bitmap when casting nested structs with non-nullable fields by aaron-seq · Pull Request #50546 · apache/arrow

aaron-seq · 2026-07-19T11:47:24Z

Rationale for this change

When casting a nested struct whose inner field changes from nullable to
non-nullable, CastStruct::Exec currently calls
in_values->GetNullCount() > 0 without considering the parent struct's
validity bitmap. This causes a spurious ArrowInvalid error when the
child's physical nulls are fully masked by null parent entries.

Reported in #50515.

What changes are included in this PR?

In scalar_cast_nested.cc, replace the unconditional rejection with a
three-way check:

Parent has no bitmap → child nulls are unmasked; reject.
Child has no bitmap → element-wise check against parent validity.
Both bitmaps present → use BinaryBitBlockCounter::NextAndNotWord()
to detect parent-valid-AND-child-null positions without heap allocation.

Are these changes tested?

Five new C++ test cases in scalar_cast_test.cc:

StructNestedNullabilityAbsentParent — true violation, must reject
StructNestedNullabilityMasked — masked null, must succeed
StructNestedNullabilitySliced — offset correctness for both outcomes
StructNestedNullabilityAbsentChild — no nulls, must succeed
StructNestedNullabilityDeep — 3-level nesting with masked null

One new Python test in test_compute.py:

test_cast_struct_nested_nullability — end-to-end PyArrow validation
covering success, rejection, and sliced array cases

Are there any user-facing changes?

Struct casts that were previously rejected with
ArrowInvalid: field '...' has nulls will now succeed when the nulls
are masked by parent-level nulls. This is a correctness fix, not a
behavior change — the previous behavior was incorrect per the Arrow
columnar format specification.

GitHub Issue: [C++][Python] Casting nested structs fails when inner struct are non-nullable, but the field for the inner struct is nullable #50515

…ct cast When casting a struct with a nullable->non-nullable field change, CastStruct::Exec checks GetNullCount() on the child array without considering the parent struct's validity bitmap. This rejects casts where the child's physical nulls are fully masked by parent-level nulls. Intersect the parent and child validity bitmaps before deciding whether unmasked nulls exist, using BinaryBitBlockCounter for the common case and explicit fallbacks for absent bitmaps.

github-actions · 2026-07-19T11:47:48Z

⚠️ GitHub issue #50515 has been automatically assigned in GitHub to PR creator.

pitrou · 2026-07-21T15:39:46Z

3. **Both bitmaps present** → use `BinaryBitBlockCounter::NextAndNotWord()`
   to detect parent-valid-AND-child-null positions without heap allocation.

You can use CountAndSetBits which does exactly that :)

pitrou · 2026-07-21T15:42:18Z

+              }
+              position += block.length;
+            }
+          }


If we get here, I think we should make sure that the final casted child does not have a null bitmap, otherwise it might violate expectations for a nullable field (which are not well specified, unfortunately).

pitrou · 2026-07-21T15:43:05Z

+            // Child has nulls but no bitmap (e.g. NullArray, RunEndEncoded, Union).
+            // We must semantically check if any valid parent element corresponds to a null child.


The BinaryBitBlockCounter path below does not account for logical nulls, so I don't think this one should, either.

We can just hardcode an error for NullArray.

pitrou · 2026-07-21T15:49:58Z

@aaron-seq @davlee1972 thanks for the issue and PR.

This is a bit tricky as we have never formalized what the nullable flag means for non-trivial types, so I've started a discussion on the dev ML to make sure we don't go into the wrong direction here:
https://lists.apache.org/thread/osgpy5b12bhyb2vpxd8brtv9zzc4cq9s

davlee1972 · 2026-07-21T18:44:58Z

I understand the complexity and for pure performance reasons I don't think you want to add Nulls in a non nullable array to begin with, but I'm forced to even out array lengths between parent records and child records.

I added a function called fill_nulls_with_dummy() as a workaround to populate any non nullable array with zeros, empty string, 1970 dates, 0 duration, etc..

Maybe what we really need is a pyarrow.notnull() type, an option in pyarrow.compute.fill_null() to use a dummy fill value based on the values type and a pyarrow compute non_null_scalar() function that takes a type and returns back a dummy scalar.

davlee1972 · 2026-07-21T19:14:14Z

I actually like how assembling list arrays work with a pure non nullable array with an offsets array which has None for null positions without having to pollute the data array with None values.

It would be a major change to support an offsets array for struct arrays.. [0,1,2,None,None,3]. For a struct array with only four non nullable array values but contains two additional null entries.

…cast - Replace the manual BinaryBitBlockCounter loop with CountSetBits/ CountAndSetBits for the case where both parent and child have validity bitmaps. - Conservatively reject (rather than doing a per-element logical-null check) when the child has no validity buffer of its own, e.g. NullType, RunEndEncoded, or Union, since a bitmap-only check can't distinguish masked from unmasked nulls there anyway. - Strip the validity buffer from a non-nullable field's cast output so masked-but-physically-null child data doesn't leak through if that field is later extracted on its own. - Add a regression test covering the no-child-bitmap path, rename the test that didn't actually exercise it, and fix a couple of lint nits (an overlong line and a missing blank line before the next test).

…ulls

aaron-seq · 2026-07-23T17:33:31Z

Thanks for the review! Pushed a follow-up commit addressing all three points:

CountAndSetBits: replaced the manual BinaryBitBlockCounter/NextAndNotWord loop with CountSetBits(parent) - CountAndSetBits(parent, child) for the case where both parent and child have validity bitmaps.
No-bitmap child (NullType/REE/Union): dropped the per-element logical-null check and now conservatively reject whenever the child reports any nulls and has no validity buffer of its own consistent with the bitmap-only semantics used in the main path, rather than mixing in logical-null reasoning.

Non-nullable field output: the cast result for a non-nullable field now has its validity buffer stripped before being attached to the parent. Previously the masked-but-physically-null bits from the source could survive into the output, which would surface as real nulls if that field were later extracted on its own (e.g. struct_field/flatten) thanks for catching that, it was the one actual correctness gap left.

Also added a regression test for the no-bitmap-child path (the existing "AbsentChild" test didn't actually exercise it, so I renamed it to reflect what it covers and added a new one), and fixed a couple of lint nits (an overlong line, a missing blank line in the Python test).

pitrou

Thanks for the update @aaron-seq ! Here are a couple more suggestions.

pitrou · 2026-07-27T14:45:30Z

+            unmasked_null_count = arrow::internal::CountSetBits(
+                                      parent_bitmap, in_array.offset, in_array.length) -
+                                  arrow::internal::CountAndSetBits(
+                                      parent_bitmap, in_array.offset, child_bitmap,
+                                      in_values->offset, in_array.length);


Can we instead add a function CountAndNotSetBits in arrow/util/bitmap_ops.h and use it directly here?

pitrou · 2026-07-27T14:46:32Z

+            // Child reports nulls but has no validity buffer of its own (e.g.
+            // NullArray, RunEndEncoded, Union). There is no cheap, bitmap-only way


This will only apply to NullArray. RunEndEncoded and Union should return 0 for GetNullCount (which should not be confused with ComputeLogicalNullCount).

pitrou · 2026-07-27T14:50:23Z

+  auto src = ArrayFromJSON(outer_type_src, R"([
+    {"inner": {"a": 1}},
+    null
+  ])");


This does not guarantee that the inner field will have nulls at all, it's just an implementation detail of ArrayFromJSON. Can you instead use StructArray::Make to construct the outer array from its inner child array?

pitrou · 2026-07-27T14:52:12Z

+TEST(Cast, StructNestedNullabilityNoChildBitmapConservativelyRejected) {
+  // NullType (and other child types without a validity buffer of their own,
+  // e.g. RunEndEncoded/Union) have no bitmap to distinguish masked from
+  // unmasked nulls, so any reported null is conservatively rejected -- even


I don't think REE or Union would be rejected, can you fix the comment?

pitrou · 2026-07-27T14:52:51Z

+  ])");
+  EXPECT_RAISES_WITH_MESSAGE_THAT(
+      Invalid, ::testing::HasSubstr("has nulls. Can't cast to non-nullable field"),
+      Cast(src, CastOptions::Safe(outer_type_dest)));


Can you also check with a zero-length input?

pitrou · 2026-07-27T14:53:30Z

+  EXPECT_RAISES_WITH_MESSAGE_THAT(
+      Invalid, ::testing::HasSubstr("has nulls. Can't cast to non-nullable field"),
+      Cast(src->Slice(1, 1), CastOptions::Safe(outer_type_dest)));
+}


Can you also check with a zero-length input? (not sure whether it should succeed or fail)

- Add arrow::internal::CountAndNotSetBits to bitmap_ops, replacing the CountSetBits/CountAndSetBits subtraction in the struct cast kernel with a single "parent valid AND child null" count, plus a direct unit test. - Drop the "no cheap way to tell" reasoning for bitmap-less children: GetNullCount() only counts physical nulls, so union and run-end encoded children report 0 and never reach the check. NullType is the only type that does, and it is always rejected. Comments updated accordingly. - Build the nested-nullability test inputs with StructArray::Make and explicit validity bitmaps so where "a" physically holds nulls is pinned down by the test instead of ArrayFromJSON's layout choices. - Cover zero-length input on both the int32 and NullType paths: an empty slice reports no nulls, so it succeeds even at the offset of a null that the full array is rejected for. - Strip trailing whitespace in test_compute.py (autopep8 lint failure).

aaron-seq · 2026-07-28T07:16:09Z

Thanks @pitrou, all five points addressed in da10b42.

CountAndNotSetBits — added to arrow/util/bitmap_ops.h/.cc (same BinaryBitBlockCounter shape as CountAndSetBits, using NextAndNotWord()), and the kernel now calls it directly instead of subtracting CountAndSetBits from CountSetBits. Added BitUtilTests.TestCountAndNotSetBits covering unaligned offsets on both sides, zero length, and the (x & y) + (x & ~y) == x identity.

Bitmap-less children — you're right, I had this backwards. GetNullCount() only counts physical nulls, so union and run-end encoded report 0 and never reach the check at all. NullType is the only type that gets there without a validity buffer, so that branch is now just part of the rejection condition, with the comment rewritten to say so.

Test construction — the nested-nullability inputs are now built with StructArray::Make from an explicit "a" child plus explicit validity bitmaps at each level, so where "a" physically holds nulls is pinned down by the test rather than being an artifact of ArrayFromJSON. This also caught that the sliced test wasn't exercising what its comment claimed: the [3, 5) slice had no null in "a" at all, so it was passing via the GetNullCount() == 0 early exit rather than through the masked-null path. Fixed the fixture so it actually hits it.

NullType test comment — rewritten; it no longer claims REE/union are rejected, and notes they report no physical nulls so they're unaffected.

Zero-length input — added on both paths. It succeeds: an empty slice reports no nulls, so the check is skipped. Worth noting the NullType case is the interesting one — src->Slice(1, 1) is rejected but src->Slice(1, 0) succeeds, since ArrayData::Slice carries null_count = len for an all-null array.

Also stripped trailing whitespace in test_compute.py, which was the autopep8 lint failure. The two red R jobs are unrelated — both are R CMD check failing on a Bioconductor index fetch (cannot open URL '.../PACKAGES'), with FAIL 0 on the actual test suites.

One thing I left alone pending the ML thread: the check runs per cast level, so if the outer element is null but the inner struct is physically valid there with a null leaf, the recursive inner cast still rejects it — it has no view of the outer bitmap. I didn't add a test asserting that either way, since it seems to be exactly what's under discussion.

aaron-seq requested review from AlenkaF, pitrou, raulcd and rok as code owners July 19, 2026 11:47

github-actions Bot added Component: C++ Component: Python awaiting review Awaiting review labels Jul 19, 2026

pitrou reviewed Jul 21, 2026

View reviewed changes

Aaron Sequeira and others added 2 commits July 23, 2026 17:25

Merge branch 'apache:main' into fix-50515-nested-struct-cast-masked-n…

7232ac3

…ulls

pitrou reviewed Jul 27, 2026

View reviewed changes

github-actions Bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Jul 27, 2026

		// Child has nulls but no bitmap (e.g. NullArray, RunEndEncoded, Union).
		// We must semantically check if any valid parent element corresponds to a null child.

		// Child reports nulls but has no validity buffer of its own (e.g.
		// NullArray, RunEndEncoded, Union). There is no cheap, bitmap-only way

Uh oh!

Conversation

aaron-seq commented Jul 19, 2026

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

github-actions Bot commented Jul 19, 2026

Uh oh!

pitrou commented Jul 21, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pitrou commented Jul 21, 2026

Uh oh!

davlee1972 commented Jul 21, 2026

Uh oh!

davlee1972 commented Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aaron-seq commented Jul 23, 2026

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaron-seq commented Jul 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

davlee1972 commented Jul 21, 2026 •

edited

Loading